sequenceDiagram
  participant EmbeddedGeneration
  participant StageOpenAiBackend
  participant InboundMessageReader
  participant StaleDiscardRegistry
  participant EdgeStage

  EmbeddedGeneration->>StageOpenAiBackend: send stale window range
  StageOpenAiBackend->>InboundMessageReader: send DiscardStaleWindows
  InboundMessageReader->>StaleDiscardRegistry: record discard range
  InboundMessageReader->>EdgeStage: forward discard message
  EdgeStage->>StaleDiscardRegistry: check window ID
  EdgeStage->>StageOpenAiBackend: send empty predicted-token reply

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Reviewed — solid, careful PR. The core design is right and the opt-in gating is done properly. A handful of things to settle before I'd approve; one is a conscious behavioral sign-off, the rest are minor.

What I checked

• skippy-protocol tests pass locally (45/45): generation bump 4→5, WireMessageKind::DiscardStaleWindows = 23, MAX_VERIFY_WINDOW_RUNAHEAD_TOKENS.
• skippy-server won't build in my env (FFI/llama.cpp libc++ <array> header failure — toolchain, not the PR), so the scheduler / reader-thread / stale_discard logic is a static review only. Those unit tests need to be green in CI before merge.

What's good

• The race is solved correctly: the reader thread records the discard range in the registry before the channel send, so even a full prefetch channel doesn't delay recording. Lookahead sized 2×depth with a clear rationale and a capacity-1 regression test.
• Reader Drop shuts the socket down and joins the thread — no leaked thread/fd, with a dedicated test.
• Malformed discards ignored ("optimization, never a correctness dependency") — good invariant, tested.
• Forward progress preserved: .max(1) guarantees at least one window admits even when a single window exceeds the budget.
• Generation bump correctly gates kind 23 so pre-gen-5 peers are excluded from split planning.

Worth addressing

1. (medium — needs a decision) The inbound reader thread + prefetch channel is spawned unconditionally for every binary connection, including fixed-depth setups. The wire is byte-for-byte unchanged, but the receive path is not: in fixed-depth mode no discards are ever sent, so is_discarded is always false and the reader adds zero correctness value — just a background thread and a prefetch channel of max(max_inflight, 128) frames (each up to MAX_STAGE_FRAME_BYTES = 8 MB) that can buffer in userspace. Steady-state it's bounded by admission so the delta is modest, but I'd either gate the reader on run-ahead mode (cleanest — keeps fixed-depth genuinely unchanged) or explicitly sign off that unconditional prefetch is intended and the worst-case memory ceiling is fine.

2. (question) Lookahead of 2×MAX_VERIFY_WINDOW_PIPELINE_DEPTH = 128 assumes ≤~128 unread frames sit ahead of a discard. That holds per request. Does a single connection ever multiplex enough concurrent requests/sessions that their summed backlogs push a discard past 128 unread frames? If so, the reader blocks on a full channel and the discard stays unrecorded until the executor drains — silently degrading back to executing the stale tail. I think per-connection admission makes this safe, but confirm.

3. (question) Skipped windows reply with an empty PredictedTokens. You note the driver's stale drain "only uses the window id for FIFO bookkeeping" — worth confirming the driver's accept-rate telemetry / token counters treat a zero-token reply for a stale window identically and don't skew the accept metric or trip an assert.

Nits
4. Docs are now inconsistent: the token is stage-generation-5 but surrounding prose in README.md, docs/skippy/DATA_FLOW.md, and docs/design/TESTING.md still says "generation 4" / "generation-4 topology" / "generation 4 is a compatibility-breaking change." Should read generation 5.
5. config_schema_defaults_ui_reference.json lost its trailing newline (diff shows "No newline at end of file"). Regen if the generator emits one.
6. run_binary_stage_message gained DiscardStaleWindows in its match arm, but discards are continued in the connection loop before execution — confirm that arm is reachable (defensive) rather than dead code.
7. WIRE_SAMPLE_COUNTER is process-global, so concurrent conditioned writers share one interleaved sample sequence rather than independent per-stream RNG. Fine for a bench model (you call it non-cryptographic) — just noting the jitter isn't independent per link under concurrency.

Nothing here is a correctness blocker on the run-ahead path itself. #1 is the one I'd want a conscious answer on; the rest are quick.

Re the out-of-diff scheduler finding (has_capacity admitting past the token budget): fixed in b122b12. The scheduler enforces the budget from the second in-flight window on (open rejects, and admissible_window_tokens reports the remaining width), and the admission loop clamps its chunk width to the remaining budget rather than planning a chunk the budget cannot fit — so in-flight tokens never exceed the budget once a window is in flight. A first window wider than the whole budget still opens from idle, so a budget narrower than one window cannot stall a request; the smoke test now caps at exactly 100/100 instead of recording 144.

On the out-of-diff finding about panics in connection workers stopping the accept loop (reap_finished turning a join Err into a bail): agreed that it is a real availability bug, but it is pre-existing on main from the iteration-scheduler work (#1420) rather than something this PR introduces or touches, so I have left it alone here to keep the diff scoped. Happy to raise it separately — say the word if you would rather it rode along with this PR.

Re-reviewed at a8eeaa70, after the four review: commits. The integration issues from my 2026-08-23 pass are addressed: run-ahead admission is now hard-bounded past the first window, the reader is joined and unblocked on drop, the discard writer is serialized with teardown, and the lookahead is decoupled from max_inflight.

Build note (updates my 2026-08-24 comment): skippy-server builds in this environment now, so the scheduler / reader-thread / stale_discard logic is no longer static-review-only.

• cargo test -p skippy-server — 536 passed, 0 failed, 3 ignored
• cargo test -p mesh-llm-host-runtime — 2663 passed, 0 failed, 8 ignored
• cargo test -p skippy-protocol -p mesh-llm-config — all green
• The new socket/timing tests (wire, message_receive, stale_discard, 17 total) ran 5x clean, no flakes

All at git rev-parse HEAD = a8eeaa70c7196e49a75a0ed554888ed8c2044991.

Four things left. None of them block the design; (1) and (2) I'd want settled before merge.

1. The inbound read-ahead ceiling is now a per-connection DoS bound, and it got much looser

spawn_message_reader runs on every binary connection, including fixed-depth setups that will never see a DiscardStaleWindows. That's fine on its own — but it moves the flow-control boundary. Before this PR, receive_next_message read on the executor thread, so an unread frame stayed in the kernel socket buffer. Now the reader parses eagerly into userspace, bounded by INBOUND_LOOKAHEAD_MESSAGES (128) or INBOUND_LOOKAHEAD_BYTES (256 MiB), whichever binds first.

For a well-behaved driver this is harmless: a depth-2 driver never has more than 2 windows outstanding, so the queue never fills. The bound matters for the case it exists to cover — a peer that sends more than it should. That ceiling went from O(max_inflight x frame) to a flat 256 MiB per connection, on a listener that accepts connections concurrently.

I don't think you need the full 256 MiB for the property you're buying. A DiscardStaleWindows frame is ~100 bytes; it overtakes a 32 MiB activation backlog exactly as reliably as it overtakes a 256 MiB one. Dropping INBOUND_LOOKAHEAD_BYTES to 32 MiB gets the same discard behaviour with an 8x smaller worst case.

2. Every simulated lane draws the identical jitter sequence

next_uniform_sample is a pure function of WIRE_SAMPLE_INDEX, which is thread-local and starts at 0 in every thread. So two threads don't get independent streams — they get the same stream. each_thread_draws_its_own_deterministic_sequence asserts exactly that (assert_eq!(first, second)).

propagation_delay() is called from run_forwarder (async_forwarder.rs:160), which is one writer thread per downstream lane. So in any topology with more than one lane, every lane takes its 40 ms burst stall on the same message index. That's a synchronized-loss model, not the contended-link model the flag is documenting — and synchronized head-of-line stalls are much easier for a pipelined scheduler to ride out than independent ones, so the jitter column in your table is likely optimistic for >2-stage splits. (For the 2-process loopback bench in the PR body, single lane, no effect — those numbers stand.)

Moving to per-thread streams was the right call for @ndizazzo's reproducibility point; it just needs a per-thread salt to also be independent. Mixing a monotonically-assigned thread ordinal into the splitmix input keeps each stream reproducible and decorrelates them. The existing test then asserts the opposite of what it does today.

3. The lookahead invariant in the doc comment isn't the one the code holds

The comment on INBOUND_LOOKAHEAD_MESSAGES says:

so this covers the whole admitted backlog: the reader never blocks on a stale window while a DiscardStaleWindows for it is still unread in the socket.

That held before the byte gate landed. It doesn't now: 64 windows at up to MAX_STAGE_FRAME_BYTES (8 MiB) is 512 MiB, past the 256 MiB ceiling, so the reader can park with the discard still unread. The failure mode is benign — it degrades to today's execute-the-stale-tail cost, and the executor keeps draining so there's no deadlock — but the comment states an invariant as unconditional when it's conditional on frame width. Worth saying "usually overtakes; falls back to executing the tail when it doesn't" rather than "never."

4. Docs and small stuff

• docs/skippy/DATA_FLOW.md: the section heading is still ## Generation 4 Direct Prediction Return and Verify Retirement while its body now says generation 5.
• More substantively, DiscardStaleWindows (kind 23) is the thing that motivated the generation bump, and it isn't described in DATA_FLOW.md at all. A compatibility-breaking generation should document the frame that broke it — including the "middle stages forward and still execute, final stage skips" rule, which is the non-obvious part and currently only lives in the PR body.
• InboundMessageReader::next's doc says "Mirrors receive_next_message's EOF classification" — that function is deleted in this PR, so the reference dangles.
• WireCondition::with_jitter runs the MAX_SIMULATED_DELAY_MS loop before the is_finite checks, so delay_ms = f64::INFINITY reports "must not exceed 3600000 ms" instead of "must be finite". Cosmetic.

One question, not a finding

The generation-5 gate lives entirely in mesh split planning (supports_skippy_stage_generation in convert.rs). The standalone serve-binary --downstream host:port path has no generation handshake — I grepped crates/skippy-server/src for STAGE_PROTOCOL_GENERATION and the generation feature tokens and found no references. So a gen-4 stage binary that receives kind 23 fails TryFrom<i32> ("unknown stage message kind") and drops the connection.

Since run-ahead is opt-in and the discard only ships in run-ahead mode, this only bites someone who turns run-ahead on across a mixed-version standalone pair — which is exactly the manually-wired path your own bench uses. Is "upgrade all stages together" the contract for standalone, or do you want the sender to degrade? Either answer is fine; I'd just like a sentence in crates/skippy-server/README.md saying which, because today the negotiation story reads as complete and it's only complete for the mesh path.

Thanks — all four addressed in b60599e.

1. Read-ahead ceiling. Agreed, and the framing is the useful part: this is a per-connection bound on what a peer can make the process buffer, not just a queue size. Dropped to 32 MiB. You're right that the discard overtakes a 32 MiB backlog exactly as reliably.

2. Identical jitter streams. This was a real bug in my fix and the test asserted the wrong property — thank you. Per-thread index alone gives every lane the same sequence, which is synchronized loss, not a contended link. Now salted with a per-thread stream ordinal claimed on first draw. uniform_sample is pure in (stream, index) so both properties are tested directly: reproducible within a stream, pairwise-distinct across streams, plus the thread-level test now asserting assert_ne!.

On the table: the loopback bench is single-lane so those numbers stand, but I've taken your point that the jitter column is likely optimistic for >2-stage splits and I'm not going to quote it for multi-stage until the lab bench runs with independent streams.

3. Lookahead invariant. Correct, and my comment overstated it. It now says the discard usually overtakes, names the byte gate as the case where it doesn't, and states the fallback (execute the stale tail — today's cost, no deadlock, because the executor keeps draining).

4. Docs. All fixed: the DATA_FLOW heading said "Generation 4" because my earlier pass only replaced the lowercase form; the dangling receive_next_message reference is gone; with_jitter checks finiteness before the magnitude bound so infinity reports what's actually wrong. DiscardStaleWindows now has its own section in DATA_FLOW.md including the middle-stages-forward-and-execute vs final-stage-skips rule, which as you say was only living in the PR body.

On the standalone question: the contract is upgrade-all-stages-together, and I've said so in crates/skippy-server/README.md — including explicitly that it is not degraded gracefully (the older peer rejects kind 23 and drops the request connection). You're right that the negotiation story read as complete when it was only complete for the mesh path.

Re-requested review. Everything from both passes is in and CI is green at b60599e7 (37/37).

Also filed the two things that came out of review but belong to #1420 rather than this PR, so they don't get lost: #1472 (the flaky worker_panic_is_contained_and_fails_active_requests race, which fail-fast-cancelled three test batches on one of this PR's runs) and #1473 (a panicking connection worker stopping the accept loop — CodeRabbit's out-of-diff finding).

Rebased onto main and re-answered both blockers. Head is 842c5811d; the branch is MERGEABLE again.

The rebase was 90 commits and moved four things this branch had to answer for, so it is worth reading as more than a conflict resolution.

1. The generation gate had to move (i386's blocker on kind 23). Main is now at generation 6, and generation 6 does not carry wire kind 23. The generation-5 bump that originally answered that thread therefore stopped separating peers that can parse the discard from peers that cannot: a current-main peer advertises stage-generation-6, clears split planning, and still rejects kind 23 as an unknown message kind. Bumped to generation 7, with split planning gating on stage-generation-7, and the docs now name kind 23 as the reason the generation breaks rather than leaving it to be inferred.

2. Infinite bandwidth quotient (i386's second blocker). bandwidth_delay returned Duration::ZERO when a positive but tiny rate overflowed the quotient to infinity, which served the slowest configurable link as an unmetered one. Now only NaN and non-positive quotients yield no delay; infinity takes the MAX_SIMULATED_DELAY_MS cap. The regression test asserts the quotient really is infinite before checking the cap, and I verified it fails against the old branch (0ns against 3600s).

3. A main fix this branch was quietly dropping. Main routes inbound reads through ConnectionWorkerControl::wait_for_readable so a worker never sits in a read that a socket shutdown cannot interrupt on Windows (#1538). Moving the read onto the inbound reader thread lost that: after the mechanical rebase, worker_control was an unused parameter. The reader now waits through the same control and ends when shutdown is requested, and the byte-ceiling backoff observes shutdown too. Worth a look, since it is the one place where the rebase changed behavior rather than just resolving text.

4. New config gates on main. verify_window_runahead_tokens now needs a website config-reference row, a wiring-manifest entry, and a reverse-audit entry. All three added.

Also added the lane-reuse teardown coverage @ndizazzo asked for: a_reused_lane_carries_the_next_request_after_a_delayed_teardown writes the next request's frame on the same socket after the delayed discard and the teardown Stop, and asserts all three arrive whole and in order. A real pooled lane carrying a real second generation still wants the two-machine harness.

Local verification on 842c5811d, native runtime built with LLAMA_STAGE_BACKEND=metal:

• cargo test -p skippy-server — 672 passed, 0 failed, 3 ignored
• cargo test -p mesh-llm-host-runtime — 2898 passed, 0 failed, 8 ignored
• cargo test -p skippy-protocol — 43 passed; cargo test -p mesh-llm-config — all green
• cargo fmt --all -- --check, all five xtask repo-consistency checks, and clippy on the touched crates — clean

Unrelated breakage found while building, not fixed here: the llama.cpp patch queue on main has three patches numbered 0035, and scripts/prepare-llama.sh applies them in sorted order. 0035-skippy-account-retained-tensors-during-staged-loads.patch (#1542) sorts first but expects context that a later patch adds, so a fresh prepare-llama.sh fails on it with sha1 information is lacking or useless. Only incremental trees that already had it applied are unaffected, which is presumably why CI is green. I built locally by renumbering it to sort last. Happy to raise it separately.

Correction to my previous comment: the llama.cpp patch-queue breakage is fixed here after all, in cfb8739e2, because it is what was failing this PR's Static ABI lane and nothing else could go green past it.

0035-skippy-account-retained-tensors-during-staged-loads.patch is renumbered to 0046 so it applies after 0038, which adds the skippy-hardware-application-probe target its src/CMakeLists.txt hunk expects. The patch bytes are unchanged; only the apply order moves. Verified by deleting .deps/llama.cpp and replaying the whole queue from a fresh clone, then building the Metal staged ABI libraries from the result.

Worth knowing for anyone else who hits it: incremental trees that already had the patch applied are unaffected, so it only bites a clean checkout. The three earlier Clippy batch failures on this PR were fail-fast cancellations from that ABI lane, not Clippy findings.

CI is green at cfb8739e2: 40 passed, 0 failed, 12 skipped.

Rebased again onto 26f9fc567 and dropped my patch-numbering commit in favour of #1587, which landed while I was pushing and fixes the same thing properly. Head is d0b1aa2f2.

Worth saying explicitly: #1587 is strictly better than what I had. I renumbered the one offending patch to 0046, which happened to collide with the tail after #1587 renumbered the whole sequence contiguously to 0047 and added validate_patch_sequence to fail fast on duplicates and gaps. My version would now fail that validator. It is gone from this branch and no trace of it remains in the diff.

Also corrected a rebase artifact of my own: resolving the console-print ratchet conflict the first time took my side wholesale and silently dropped main's entries for crates/skippy-bench/src/direct_return_listener.rs from #1560. Regenerated, and the ratchet now differs from main in exactly one file, binary_messaging.rs, which is the only file this PR shifts lines in.

The mid-frame shutdown fix from 981d9cc32 carried across the rebase unchanged.

Re-verified at d0b1aa2f2 with the native runtime rebuilt against the new patch queue:

• cargo test -p skippy-server — 665 passed, 0 failed, 3 ignored
• cargo test -p mesh-llm-host-runtime — 2886 passed, 0 failed, 8 ignored
• cargo test -p skippy-protocol -p mesh-llm-config — green
• cargo fmt --all -- --check and all five xtask repo-consistency checks — clean

The suite counts moved against my previous comment because main did (#1588 derives KV policy from the loaded model), not because anything here changed.

Rebased onto 12c83e83b and resolved the conflicts. Head is b6b1aecb7, MERGEABLE again.

The generation boundary moved a third time. Main is now at generation 7 itself, via #1585, and generation 7 does not carry wire kind 23. That is the same situation as last round, one generation later: a generation-7 peer advertises current support, clears split planning, and still rejects the discard frame as an unknown message kind. So this branch now bumps to generation 8, keeping main's wording for the rest of the generation contract (the control, status-list, and strict-content-identity bundle) and adding kind 23 to it.

Worth flagging as a pattern rather than a one-off: this is the third rebase where the only substantive change was moving the generation number, because the branch and main keep claiming the same one. @i386, if you would rather kind 23 rode on a dedicated capability token instead of the generation counter, that is the route you offered at the start of the original thread and it would stop this recurring. I have not switched to it unilaterally, because it needs plumbing from mesh split planning into the embedded frontend's send path and that is a bigger change than a rebase should carry. Say the word either way.

Other conflict resolutions, none of them behavioral:

• Make Skippy negotiate real activation boundaries #1585 split the connection's activation width into input and output halves. The inbound reader parses upstream frames, so it takes the input width.
• README, DATA_FLOW.md and TESTING.md kept main's generation wording, with the kind-23 sentence and the standalone-path contract paragraph merged back in rather than overwriting either side.
• Console-print ratchet regenerated; it differs from main in exactly one file, binary_messaging.rs, which is the only file this PR shifts lines in.

Re-verified at b6b1aecb7 with the native runtime rebuilt against main's current patch queue:

• cargo test -p skippy-server — 674 passed, 0 failed, 3 ignored
• cargo test -p mesh-llm-host-runtime — 2899 passed, 0 failed, 8 ignored
• cargo test -p skippy-protocol -p mesh-llm-config — green
• python3 scripts/tests/test_prepare_llama.py — 3 passed, so the branch is clean against fix(ci): restore deterministic llama patch replay #1587's new patch-sequence validator
• cargo fmt --all -- --check and all five xtask repo-consistency checks — clean

CI caught one thing the generation-8 bump broke, fixed in bbc69cd54.

stage_control_request_validates_generation_sender_and_command builds a frame at STAGE_PROTOCOL_GENERATION - 1 but asserted the rejected value against the literal 6, so it failed the moment the constant moved. The other three red checks were downstream of it: batch-3 was fail-fast cancelled, and the two Enforce Linux result gates aggregate the batch.

It now asserts against the computed previous generation instead of a literal, so it keeps testing what it means to test (the previous generation is rejected) rather than failing on the number at every bump. That is the same recurring cost I flagged in my last comment, in test form.

My own miss, worth owning: I ran that suite locally before pushing and my output filter cut off above the failing line, so I reported it green when it was not. The full suites at bbc69cd54:

• cargo test -p skippy-protocol — 43 passed, 0 failed
• cargo test -p skippy-server — 674 passed, 0 failed, 3 ignored
• cargo test -p mesh-llm-host-runtime — 2899 passed, 0 failed, 8 ignored
• cargo test -p mesh-llm-config — 188 + 4 + 5 + 10 passed, 0 failed
• cargo fmt --all -- --check — clean